Skip to content

Conversation

@OleksiienkoMykyta
Copy link
Contributor

@OleksiienkoMykyta OleksiienkoMykyta commented Aug 1, 2024

Cassandra 4.0 has some minor table metadata changes that need to be accounted for:
read_repair_chance is replaced with read_repair
dclocal_read_repair_chance has been removed
additional_write_policy has been added

In the PR implemented backward compatibility with previous versions, and added new types support. To make metadata table support easier for future Cassandra versions, hardcode scan from Cassandra were replaced with new "parseSystemSchemaViews" method which is much easier to expand, even if some fields were added in the middle of the table it wouldn`t be an issue anymore.

MemtableFlushPeriodInMs: 0,
MinIndexInterval: 128,
ReadRepair: expectedReadRepair,
ReadRepairChance: 0,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, read_repair_chance was removed from C* 4.0.

https://issues.apache.org/jira/browse/CASSANDRA-15604

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but this test also runs for C* 3.0.0, so it covers data for all versions. We can make separate tests for each version, but as mentioned in the description for the current PR, the main aim is to implement a backward-compatible metadata getter, and this test case helps to test one.

@OleksiienkoMykyta OleksiienkoMykyta force-pushed the add-support-for-Cassandra-5-0-table-options branch 2 times, most recently from 35cc199 to 58924f7 Compare August 7, 2024 08:33
metadata.go Outdated
Comment on lines 980 to 987
func extensionsBlobToStrings(value interface{}) (map[string]string, error) {
const errorMessage = "gocql.extensionsBlobToStrings failed to read column %s"
byteData, ok := value.(map[string][]byte)
if !ok {
return nil, fmt.Errorf(errorMessage, "extentions")
}

extensions := make(map[string]string, len(byteData))
for key, rowByte := range byteData {
extensions[key] = string(rowByte)
}

return extensions, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func extensionsBlobToStrings(value interface{}) (map[string]string, error) {
const errorMessage = "gocql.extensionsBlobToStrings failed to read column %s"
byteData, ok := value.(map[string][]byte)
if !ok {
return nil, fmt.Errorf(errorMessage, "extentions")
}
extensions := make(map[string]string, len(byteData))
for key, rowByte := range byteData {
extensions[key] = string(rowByte)
}
return extensions, nil
}
func bytesMapToStringMap(bytesMap map[string][]byte) map[string]string {
stringMap := make(map[string]string, len(bytesMap))
for key, bytes := range bytesMap {
stringMap[key] = string(bytes)
}
return stringMap
}

Let's make this method take map[string][]byte as a param instead of an empty interface. This approach will allow you to return described in this func error more properly from materializedViewMetadataFromMap.
Also, what do you think about renaming this func to something like bytesMapToStringMap?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, it will make bytesMapToStringMap more reusable and error handling much more readable.
Thank You very much!

@OleksiienkoMykyta OleksiienkoMykyta force-pushed the add-support-for-Cassandra-5-0-table-options branch from 58924f7 to e28b00a Compare August 7, 2024 11:36
@OleksiienkoMykyta OleksiienkoMykyta changed the title Add support for cassandra 5.0 table options Add support for cassandra 4.0 table options Oct 29, 2024
@joao-r-reis joao-r-reis changed the title Add support for cassandra 4.0 table options CASSGO-13 Add support for cassandra 4.0 table options Oct 29, 2024
metadata.go Outdated
MaxIndexInterval int
MemtableFlushPeriodInMs int
MinIndexInterval int
ReadRepair string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a small comment here and ReadRepairChance to indicate that ReadRepairChance was removed in C* 4.0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like ReadRepairChance float64 // Deprecated since Cassandra v4, use ReadRepair instead.?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like:

ReadRepair              string    // Only present in Cassandra 4.0+
ReadRepairChance        float64   // Note: Cassandra 4.0 removed ReadRepairChance and added ReadRepair instead

@OleksiienkoMykyta OleksiienkoMykyta force-pushed the add-support-for-Cassandra-5-0-table-options branch from daaa2bc to c171403 Compare December 11, 2024 10:59
@joao-r-reis
Copy link
Contributor

Can you resolve the conflict and add James to the commit message?

In the PR implemented backward compatibility with previous versions,
and added new types support. To make metadata table support easier for
future Cassandra versions, hardcode scan from Cassandra were replaced
with new "parseSystemSchemaViews" method which is much easier to expand,
even if some fields were added in the middle of the table it wouldn`t be an issue anymore.

patch by Mykyta Oleksiienko; reviewed by Joao Reis and James Harting CASSGO-13
@OleksiienkoMykyta OleksiienkoMykyta force-pushed the add-support-for-Cassandra-5-0-table-options branch from c171403 to 20b0e6b Compare February 5, 2025 10:16
@joao-r-reis joao-r-reis merged commit f3d13d4 into apache:trunk Feb 5, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants